fix: avoid blocking waiting callers during IAsyncInitializer initialization - #6906

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

Merge Risk: 🔵 Low · up to f35b8

Initialization remains usable, but an initializer that returns null produces the wrong cached exception. This bounded issue can be accepted for merge or corrected before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to f35b8

The change improves concurrent waiting without showing a new external security boundary. A recursive initializer may, however, leave its shared initialization pending instead of failing promptly. The effect on ordinary test execution remains uncertain.

Retained concerns

• Medium · reliability · inferred: A same-object recursive initializer can await its own published completion task, leaving initialization and its other waiters pending rather than failing promptly as the prior lazy factory did. Production reachability has not been established.

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit watched one task take flight,
While waiting callers shared its light.
One start, one result to pass,
Failures stayed within the task’s grasp.
Through gates and waits, the tests took care,
Then hopped away with time to spare.}

_{Comment @coderabbitai help to get the list of available commands.}

Confidence Score: 5/5

[Medium risk] Changes how object initialization tasks are cached and awaited.

The PR appears safe to merge based on the reviewed changes and resolved prior threads.

Summary

The PR replaces lock-held Lazy<Task> initialization with a shared task published before initializer code runs. The latest revision schedules waiting continuations asynchronously and adds a regression test for their execution thread.

• Caller cancellation remains separate from the cached initialization result.
• No new actionable issue was identified in the changes since the previous review.

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Caller requests initialization] --> B{Shared task exists?}
  B -->|No| C[Publish completion task]
  C --> D[Run initializer]
  D --> E[Complete shared task]
  B -->|Yes| F[Await shared task]
  C --> F
  E --> F
  F --> G[Caller observes result or cancels its own wait]

_{Reviews (3) · Last reviewed commit: "fix: queue shared initializer continuati..."}

Review

Nice fix for the thread-pool starvation described in #6904 — publishing the task before running InitializeAsync() (instead of holding a lock via Lazy<Task>) is the right shape, and the single-flight logic itself (TryGetValue/GetOrAdd/ReferenceEquals, per-caller cancellation via WaitAsync(cancellationToken), caching faulted tasks per #4715, preserving the original exception including OperationCanceledException) all check out. The dual-mode constraint doesn't apply here since ObjectInitializer sits on the shared post-discovery execution path.

One correctness issue: waiters are not guaranteed to resume off the completing thread

src/TUnit.Core/ObjectInitializer.cs line 120:

var completionSource = new TaskCompletionSource<bool>();

This is missing TaskCreationOptions.RunContinuationsAsynchronously. Without it, completionSource.SetResult/SetException in RunInitializerAsync (line 138) runs every waiting caller's continuation synchronously, one after another, on the thread that completes initialization, rather than queuing them. For a fixture shared across many tests (the PR's own '10,000 tests sharing one fixture' benchmark, and generally any ClassDataSource with Shared = PerTestSession/PerClass), that reintroduces a variant of the exact pool-pressure problem this PR sets out to fix — just moved from lock contention to inline continuation fan-out on one thread.

This also contradicts the PR's own description ('Because the source uses RunContinuationsAsynchronously, their continuations are queued rather than run one after another inline on the thread that completed the initialization') and its benchmark harness, which does construct the TaskCompletionSource with that option. The sibling single-flight cache in src/TUnit.Engine/Services/ObjectLifecycleService.cs (EnsureInitializedAsync, ~line 470) already does this correctly, so this looks like an accidental omission introduced in the latest revision rather than a deliberate change — CodeRabbit's architecture review and Greptile both flagged the same thing independently on commit 92c54e2, and it doesn't look addressed yet.

Suggested fix:

var completionSource = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

Why this needs a test, not just a fix: none of the 13 new cases in tests/TUnit.UnitTests/ObjectInitializerTests.cs pin the 'waiters resume asynchronously, not inline' guarantee (only main's 4 known-bug cases and the null-task case are covered) — the PR description lists 'waiters resume inline on the completing thread' as one of the three behaviors the new tests catch on main, but there's no corresponding assertion in the diff. Since this is the specific property that made the last row of the benchmark table faster, and it's also the easiest one to silently regress again, it's worth a deterministic test (e.g. assert the completing thread differs from an awaiting caller's captured thread, or drive it via a captured SynchronizationContext/work-item count the way the benchmark already does) rather than relying on review alone to catch its absence.

Minor, already flagged by Greptile

The PR description says a null-returning InitializeAsync() now yields 'one InvalidOperationException naming the type' for every caller, but the current RunInitializerAsync will instead let await null.ConfigureAwait(false) throw a bare NullReferenceException (still shared across all callers, just less descriptive than what's described). Not a correctness bug — the failure is still cached and shared as intended — but worth reconciling the PR description/tests with actual behavior, or restoring the descriptive exception if that diagnostic was intentional.

Thanks for the review. I've pushed the simplification in 92c54e2: one helper runs initialization, every caller awaits the same published task, and the special winner path, StartInitializer, custom null-task exception, RunContinuationsAsynchronously, and explicit exception observation have been removed. The tests now cover the five scenarios you suggested, with nine parameterized cases.

I kept one difference from the proposed snippet: InitializeCoreAsync is still async ValueTask. With a direct ValueTask return, an initializer's OperationCanceledException stored through SetException exposes Faulted rather than the previous Canceled status. StaticPropertyHandler passes these operations through .AsTask() into Task.WhenAll, so this can affect mixed cancellation/failure handling. Both cancellation-status cases fail without the wrapper. The reported allocation cost for the genuinely asynchronous per-test case is 78-80 B per first initialization versus main.

There is also a performance tradeoff worth reviewing before merging. The local end-to-end report uses 1,000 tests sharing one fixture and reports these medians over five alternating runs:

The report observed the current PR's test execution continuing on one thread in these shared-fixture cases. For asynchronous completion this already occurs on main; for blocking followed by synchronous completion, it is a regression compared with main. The original #6904 reproduction is fixed in both revised variants.

The waiter-only Task.Yield() experiment is not in this branch. Would you prefer addressing the synchronous-completion regression in this PR or discussing that scheduling change separately?

The benchmark sources, commands, and raw reports are now published at a pinned commit. The candidate is also available as a two-file diff on top of this PR. It skips yielding for the initiating caller and already completed tasks; other waiters yield after waiting. This schedules a continuation through the current context/scheduler rather than promising a separate thread per waiter.

There is a cost to evaluate: the single-worker asynchronous microbenchmark records 2,873 ns / 526 B for main, 3,082 ns / 604 B for this PR, and 3,417 ns / 623 B for the candidate. The shared-fixture improvement should not be described as universally free.

The candidate's reported unit tests passed 392/392 on net8/9/10, but full engine and NativeAOT validation still needs to run on 6f1c971; the full validation reported for 92c54e2 does not cover it. I checked the published source and reports in this follow-up, without rerunning the benchmarks.

The PR description now distinguishes the current implementation, the separate candidate, and the validation evidence. Windows and Ubuntu CI passed for the current PR; the macOS timeout-test failure is being retried. Upstream test workflows still require approval.

Thanks for the thorough benchmarking. On the open question: I'd like this addressed in this PR, and I think RunContinuationsAsynchronously is the right fix rather than the waiter Task.Yield() candidate.

var completionSource = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

Why it's needed

Without it, SetResult/SetException in RunInitializerAsync runs every registered waiter's continuation inline, one after another, on the thread that completes initialization. Each of those continuations is the rest of a test up to its next real await, so N tests sharing a fixture execute their synchronous portions serially on a single thread. Your own table shows this: "blocks, then completes synchronously" regresses from 1,425 ms on main to 2,998 ms, because waiters register while the prefix is blocked and the initiator's SetResult then drains all of them inline.

The completing thread can also be one we don't control. With ConfigureAwait(false), RunInitializerAsync resumes on whatever thread completed the user's task (a container/process event callback, a dedicated thread, etc.), and test code would then run on it. We just fixed the same class of bug in DedicatedThreadExecutor (#6898).

Why it's preferable to the waiter-yield candidate

• Zero cost when no continuations are registered at completion time, i.e. synchronous completion and the already-completed fast path.
• Only pays a thread-pool hop per waiter in exactly the case where we want fan-out.
• Composes with WaitAsync(cancellationToken): each caller's cancellation promise is completed from its own queued work item, so resumption stays parallel. With CancellationToken.None, WaitAsync returns the same task and each awaiter is queued individually.
• A one-line change, with no per-caller yield logic or extra state.
• Doesn't affect the Canceled-vs-Faulted handling, so the async ValueTask wrapper stays as is.

The trade-off is one extra pool hop for the initiating caller when InitializeAsync genuinely completes asynchronously, once per object. That's small compared to the serialized fan-out it removes, and it matches what ObjectLifecycleService.EnsureInitializedAsync already does.

Test

Please add a deterministic test for this guarantee: block the prefix, register several waiters, let initialization complete synchronously, and assert that no waiter continuation ran on the completing thread before SetResult returned (e.g. record Environment.CurrentManagedThreadId in each waiter, or have waiters check a flag set around the completion call). It should fail on 92c54e2.

Review of PR 6906. The fix is sound. Publishing a TaskCompletionSource task via GetOrAdd before any user code runs removes the lock that starved the pool in issue 6904. I checked these points:

• Exactly-once: only the caller whose GetOrAdd returns its own completionSource.Task runs InitializeAsync. The ReferenceEquals check is the right winner test.
• Exceptions: anything thrown, whether synchronously (including the null-task NullReferenceException) or asynchronously, is captured by the async Task helper and passed to SetException. The task is never left incomplete.
• Cancellation: the caller's token only cancels that caller's WaitAsync. Keeping InitializeCoreAsync as async ValueTask preserves the Canceled status that StaticPropertyHandler/Task.WhenAll consumers rely on.
• Failure caching: failures stay cached, so issue 4715 is not regressed.
• IsInitialized: the check is correct.

The tests cover the regression, contention, cached failures, and both cancellation directions, and each test's purpose is clear.

Suggestions

1. The PR description is out of date. It says there is no RunContinuationsAsynchronously and lists five test methods, but the diff uses TaskCreationOptions.RunContinuationsAsynchronously and adds a sixth test, Waiting_Continuations_Do_Not_Run_On_The_Initializing_Thread. The description also says inline continuation execution is allowed, which contradicts that code and test. I'd update the description so reviewers and later readers aren't misled. I'd also keep RunContinuationsAsynchronously: without it, waiter continuations would run on the initializing thread, inside SetResult, and could stall it.
2. Synchronous same-object re-entry now deadlocks silently. Lazy threw InvalidOperationException on factory recursion. Here the re-entrant caller awaits a task that its own initializer must complete, so it hangs. The PR acknowledges this. I'd add a one-line remark on ObjectInitializer that re-entry is unsupported, so it is discoverable if someone later hits a hang.
3. Minor: the TryGetValue before GetOrAdd is a fast path. That is fine, because it avoids allocating a TCS on the hot path once the task exists. A short comment saying so would keep someone from simplifying it away.
4. Test robustness (non-blocking): some tests wait on real time (HangTimeout, 10s) to detect a hang. That only costs time on failure, which is acceptable. The completedWhilePrefixBlocked = waiter.IsCompleted assertion in the first test is slightly racy in the passing direction only, so it should not cause flakiness.

Overall this is a good, focused change and I'd approve it once the description is corrected.

Implemented in f35b875f. I agree with keeping this guarantee at the shared completion source: it is simpler than adding waiter-side yield logic.

The completion source now uses RunContinuationsAsynchronously. The single initialization helper, shared waiting path, and async ValueTask wrapper remain unchanged.

The new deterministic test blocks the synchronous prefix, registers four waiters, then allows initialization to complete synchronously. The initiating call runs on a dedicated LongRunning thread using TaskScheduler.Default, so a correctly queued continuation cannot later reuse that same thread. Each waiter records its thread ID. The two cases cover CancellationToken.None and a cancellable token. Gates and awaits are bounded for failure cleanup; there are no elapsed-time assertions.

Validation on Windows x64 / SDK 10.0.401:

• RED: both new cases fail on 92c54e2 on net8/9/10, at the thread-ID assertion. The other nine initializer cases pass on each framework.
• GREEN: all 392 unit tests pass on each of net8/9/10 (1,176 passed total), including the cancellation-status and exception-identity checks.
• TUnit.Core builds successfully across its target frameworks with no warnings.
• NativeAOT publication succeeds. It reports warnings elsewhere in the test application/dependencies; none reference either changed file.
• The full engine run with reflection and NativeAOT enabled recorded 463 passed, 4 failed, and 10 skipped. Two failures were missing F#/VB example builds; both tests passed after building those prerequisites. The other two are ExpectedStateTests.Pass in the two modes, caused by assertions in the existing OrderedSetupTests / OrderedByAttributeOrderParameterSetupTests. I reproduced both underlying tests failing with the unchanged 92c54e2 production code in this checkout. Their output order matches; LF/CRLF is the leading explanation, not a proven new regression. I have left those tests outside this change.

I also ran a short before/after microbenchmark using the already published harness. Final_MaintainerShapeAsync models 92c54e2; PlainTcs_WinnerAwaitsSource matches the helper shape with the new flag. This is a standalone model, not an end-to-end run of the new commit. The single-worker asynchronous mean increased by about 0.40 microseconds; the synchronous 16-worker case also increased in this run. I am not claiming zero total overhead.

dotnet run -c Release -- --filter 'InitBenchmarks.Final_MaintainerShapeAsync*' 'InitBenchmarks.PlainTcs_WinnerAwaitsSource*' --launchCount 1 --warmupCount 3 --iterationCount 8 --iterationTime 150 --unrollFactor 1 --artifacts results

BenchmarkDotNet v0.15.8, Windows 11 (10.0.26200.9457/25H2/2025Update/HudsonValley2)
AMD Ryzen 9 9950X3D 4.30GHz, 1 CPU, 32 logical and 16 physical cores
.NET SDK 10.0.401
  [Host]     : .NET 10.0.12 (10.0.12, 10.0.1226.42308), X64 RyuJIT x86-64-v4
  Job-EDVFOA : .NET 10.0.12 (10.0.12, 10.0.1226.42308), X64 RyuJIT x86-64-v4

IterationCount=8  IterationTime=150ms  LaunchCount=1  
MaxIterationCount=60  MinIterationCount=30  UnrollFactor=1  
WarmupCount=3  

Thanks @Sing303 !